Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node_contextify: do not incept debug context #4819

Merged
merged 1 commit into from
Feb 5, 2016

Conversation

MylesBorins
Copy link
Contributor

Currently a debug context is created for various calls to util.

If the node debugger is being run the main context is the debug
context. In this case node_contextify was freeing the debug context
and causing everything to explode.

This change checks to see if there is currently a debug context
before setting up / tearing down a temporary context.

@mscdex
Copy link
Contributor

mscdex commented Jan 22, 2016

Can you also add a test for this?

ScopedEnvironment env_scope(debug_context, env);
const int index = Environment::kContextEmbedderDataIndex;
if (!debug_context->GetAlignedPointerFromEmbedderData(index)) {
ScopedEnvironment env_scope(debug_context, env);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is right? env_scope's destructor will get called as soon as execution moves outside of the if statement. I'm not sure, but we may need to have an if-else and copy the code after the current if into both blocks (one with env_scope and one without)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up. I have not used structs with this pattern before and was unaware of the semantics.

@mscdex mscdex added debugger vm Issues and PRs related to the vm subsystem. labels Jan 22, 2016
@MylesBorins
Copy link
Contributor Author

@mscdex I've attempted a different approach, does this solve your original concerns?

@MylesBorins MylesBorins force-pushed the do-not-double-debug branch 2 times, most recently from 6608d57 to c9f8309 Compare January 22, 2016 19:50
@@ -246,14 +246,22 @@ class ContextifyContext {
// there is no way for the embedder to tell if the data index is
// in use.
struct ScopedEnvironment {
bool alreadyInDebugContext = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: C++ code uses snake_case for locals.

EDIT: And data members use snake_case and end in an underscore: bool already_in_debug_context_ = false; Also, it should go near the definition of context_.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased in those style changes in case we decide to move forward with 803ca9667a8dc608c5271633e42e792dbba66fc0

@MylesBorins
Copy link
Contributor Author

@bnoordhuis I've just pushed another option for handling this problem.

@ofrobots was saying that my second attempt would cause a FATAL ERROR if the index slot didn't exist

The current approach, originally done as a patch shared by ofrobots does away with that, but no longer cleans up the debugger context, which may be undesirable

@@ -0,0 +1,3 @@
var util = require('util');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'use strict'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bah... didn't mean to check that in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

killed that file.

@bnoordhuis
Copy link
Member

LGTM with a nit and assuming the CI likes it.

@MylesBorins
Copy link
Contributor Author

@bnoordhuis can I get you to sign off one more time on the current head. Just want to ensure that is the one you were ok with

@bnoordhuis
Copy link
Member

LGTM but is there a reason you're not deassigning the context slot afterwards?

@MylesBorins
Copy link
Contributor Author

the deassignment was what was causing the original problem

MylesBorins@724fb28 offers an approach with deassignment but causes the debug errors mentioned above. TBQH I am a little out of my depth here, and not sure the best way to only deassign if there was not already a debugger context

@bnoordhuis
Copy link
Member

What worries me a little is that you're essentially leaving a dangling pointer. If you have a simple regression test, I can take a look.

@ofrobots
Copy link
Contributor

@bnoordhuis See: #4815. The original problem is that there is code in util.js (e.g. inspectPromise) that now keeps a keep a debug context around forever. Once that context has been created, subsequent calls to RunInDebugContext clear the environment from the debug context when they shoudn't.

I am worried about the dangling pointer, but I think Environment lives longer as the debug context so I am not sure it really is dangling?

@bnoordhuis
Copy link
Member

Not now, no, but it's a future hazard. Change itself LGTM but a regression test would be good.

@MylesBorins
Copy link
Contributor Author

@bnoordhuis are there similar regression tests I can look at to base it off of?

@bnoordhuis
Copy link
Member

Maybe the test from 25776f3?

@MylesBorins
Copy link
Contributor Author

So the edge case does not seem to show up if debugger is being run in interactive mode.

Not having a ton of luck here

@jasnell
Copy link
Member

jasnell commented Jan 23, 2016

Yeah, this one is a hard one. The change LGTM but it leaves me a bit uneasy. Unfortunately I haven't had any luck coming up with an idea for a reliable regression test either.

@MylesBorins
Copy link
Contributor Author

ping @bnoordhuis

@MylesBorins MylesBorins assigned bnoordhuis and unassigned MylesBorins Feb 3, 2016
proc.stderr.setEncoding('utf8');

function fail() {
assert(false, 'the program should not hang');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: common.fail()

@Trott
Copy link
Member

Trott commented Feb 5, 2016

Test LGTM. Would be great if @bnoordhuis or @indutny or someone else knowledgable about the C++ side of things could give an LGTM. (Ben gave one, but it was 13 days ago and I'm pretty sure the code has changed since then.)

@indutny
Copy link
Member

indutny commented Feb 5, 2016

LGTM if CI is happy.

// exploding but is still not an elegant solution. Likely a deeper bug
// causing this problem.
proc.stdin.on('error', (err) => {
console.error(new Error(err));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err is already an Error, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@bnoordhuis
Copy link
Member

LGTM with teeny tiny nits. Can I suggest you stress-test the test besides doing the regular CI?

@MylesBorins MylesBorins force-pushed the do-not-double-debug branch 2 times, most recently from 9b84a8c to 54b8693 Compare February 5, 2016 22:41
@MylesBorins
Copy link
Contributor Author

Stress Test: https://ci.nodejs.org/job/node-stress-single-test/415/

edit: stress test had over 1400 positives and 0 negatives so I stopped it.

Currently a debug context is created for various calls to util.

If the node debugger is being run the main context is the debug
context. In this case node_contextify was freeing the debug context
and causing everything to explode.

This change moves around the logic and no longer frees the context.

There is a concern about the dangling pointer

The regression test was adapted from code submitted by @3y3 in nodejs#4815

Fixes: nodejs#4440
Fixes: nodejs#4815
Fixes: nodejs#4597
Fixes: nodejs#4952

PR-URL: nodejs#4815

Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins
Copy link
Contributor Author

landed in 4897f94

@MylesBorins MylesBorins merged commit 4897f94 into nodejs:master Feb 5, 2016
@MylesBorins MylesBorins deleted the do-not-double-debug branch February 8, 2016 18:46
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 16, 2016
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - nodejs#4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - nodejs#4328
* node_contextify: do not incept debug context (Myles Borins)
  - nodejs#4819
* deps: update to http-parser 2.5.2 (James Snell)
  - nodejs#5238
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - #4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - #4328
* node_contextify: do not incept debug context (Myles Borins)
  - #4819
* deps: update to http-parser 2.5.2 (James Snell)
  - #5238

PR-URL: #5200 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants